Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lint check #2736

Closed
wants to merge 1 commit into from
Closed

lint check #2736

wants to merge 1 commit into from

Conversation

hannesa2
Copy link
Contributor

To improve code quality, I recommend a lint check on every pull request.
Currently it's obvious disabled, because on master it fails ! Please enable lint check on CI

With this pull request you see in root the lint report lint-app-report.html
image

Btw, a lot of them should be solved later on, and after that step, you should prevent them in the future by severity="error" in lint.xml

@davigonz
Copy link
Contributor

davigonz commented Nov 29, 2019

Thanks @hannesa2 , this is a good step after merging the modularization into master, I will put it inside new architecture epic

@hannesa2
Copy link
Contributor Author

hannesa2 commented Nov 29, 2019

Why not now ?
I at least see no reason to postpone it

@davigonz
Copy link
Contributor

Why not now ?

Many changes have taken place during modularization, a bunch of new files have been created, old files have been removed. If we move forward the lint check before merging modularization we could have lint suggestions that are already solved.

I'd rather include lint check and start fixing the suggestions after the huge structural change of modularization. Besides, the modularization merge into master should not take long, the last PR is ready to code review => #2728

@hannesa2
Copy link
Contributor Author

Interesting point of view.
To resume it: This PR has no blocking lint issues, I solved all blockers in this PR !

Knowing this, there are two ways:
A: merging this PR now, and you have to solve your hypothetical lint by yourself
B: merging this PR after your merge, and I've to solve your hypothetical lint by me in this PR

To be honest, that's cheeky

@davigonz
Copy link
Contributor

davigonz commented Dec 2, 2019

A: merging this PR now, and you have to solve your hypothetical lint by yourself
B: merging this PR after your merge, and I've to solve your hypothetical lint by me in this PR

To be honest, that's cheeky

  1. Please stop using sarcasm and these your , I've accusations, are disrespectful and being respectful is one of the main points of our community code of conduct. Besides, using "cheeky" is out of place here, specially when some of your PRs have not been finished by yourself - applying requested changes or fixing reported bugs and that's what every contributor or developer we have had has always done with no complains - and we have helped you to move them forward, last example => Better logging gui #2725.

  2. No one said you have to solve the lint suggestions in this PR after merging modularization, sorry if I did not explain myself properly. My idea was taking advantage of this PR and applying the lint suggestions of modularization here by ourselves, in the different modules and so on... addressing all the work related to the lint here and not splitting it up into different issues.

To sum up, I do not want to delay more the modularization merge into master and merging this PR before will block it, since the modularization build will fail because of lint errors (see below) and no modularization merge will be possible till fixing the lint errors.

Screenshot 2019-12-02 at 12 59 44

Therefore, just after merging the modularization into master, we will enable the lint check via this PR y start to work on it (I will do it, no problem).

Copy link
Contributor

@abelgardep abelgardep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments here @hannesa2 . This is related with #2750, so ill fix there every lint error related with new modularization.

.gitignore Show resolved Hide resolved
.gitignore Show resolved Hide resolved
owncloudApp/lint.xml Show resolved Hide resolved
@abelgardep abelgardep mentioned this pull request Dec 11, 2019
@abelgardep
Copy link
Contributor

Will be continued on #2750

@abelgardep abelgardep closed this Dec 11, 2019
@jesmrec jesmrec removed the Sprint label Jan 7, 2020
@hannesa2 hannesa2 deleted the LintCheck branch May 11, 2020 05:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants